fix: bound native scan claim queue reads - #3662
Patrick-Erichsen merged 1 commit into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Codex review: needs maintainer review before merge. Reviewed September 14, 2026, 5:47 PM ET / 21:47 UTC (Revision 4). ClawSweeper reviewWhat this changesThe PR sizes scan-queue pages to remaining worker capacity, expands them when blocked legacy jobs require it, and adds regression tests and a documented invariant. Merge readiness✅ Ready for maintainer review The fix remains useful: current main still performs the oversized reads. No blocking patch defects were found, and the supplied local Convex measurements support the narrowly stated read-budget improvement. Priority: P2 Review scores
Verification
How this fits togetherClawHub’s scan queue assigns pending skill and package security scans to authenticated workers. Queue selection applies source priorities and rollout restrictions before issuing leases for artifact inspection. flowchart TD
A[Worker requests capacity] --> B[Authenticate worker]
B --> C[Read ready queue pages]
C --> D[Check rollout and catalog eligibility]
D -->|Blocked legacy jobs| C
D -->|Eligible jobs| E[Issue scan leases]
E --> F[Inspect skill or package artifacts]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep capacity-sized native reads and the existing legacy fallback, while qualifying any later concurrency increase through measured production errors and successful scan throughput. Do we have a high-confidence way to reproduce the issue? Yes: current main explicitly requests 512 queue entries for small native claims when rollout is disabled, and the supplied local Convex results exercise that case. This review verified the source path without executing it. Is this the best way to solve the issue? Yes, for the read-amplification defect: changing the shared pagination owner is narrower than altering worker capacity or introducing new indexes, while retaining the broad legacy fallback avoids excessive small queries. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 9be7637941d8. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
This comment has been minimized.
This comment has been minimized.
18ddd2c to
6c6bbf2
Compare
6c6bbf2 to
2c7c10c
Compare
2c7c10c to
4b95675
Compare
Native ClawScan workers requesting one job currently fetch up to 512 ready queue rows when generic GitHub rollout is disabled. Start each source page at the worker's remaining capacity, expanding to the existing bounded window only after encountering blocked legacy GitHub jobs. Preserve priority ordering, rollout restrictions, catalog admission and lease behavior.
Before/after proof on a disposable local Convex backend (
http://127.0.0.1:3318) with 600 queued native bulk-rescan jobs:The existing 513-blocked-GitHub-job fallback still reaches the allowed NVIDIA job in at most three source pages. All 117 security-scan tests pass after rebasing onto current main. Temporary seed/proof functions were deployed only locally, excluded from this PR, and the disposable backend was stopped.
This reduces demonstrated read amplification; it is not yet a demonstrated throughput fix. Local burst tests with 128 one-job requests at concurrency 16 and 32 still encountered heavy OCC contention before and after, with no consistent throughput improvement. The production claim-error cause remains unverified. Under renewed September 14 authorization, deploy this bounded read fix and measure actual claim failures and successful scan throughput before qualifying higher capacity. Scans continue at reduced concurrency during repair; raw claim errors still count toward backoff thresholds.
Validation:
bunx vitest run convex/securityScan.test.ts— 117 passed; new regression cases fail on the original code.bun run ci:unit— 6,677 passed, 3 skipped; coverage gate passed.bun run ci:static— passed.bun run ci:types-build— passed.bunx tsc -p convex/tsconfig.json --noEmit— passed.